Erase query cache values #109333

Zoxc · 2023-03-19T00:27:10Z

This replaces most concrete query values V with MaybeUninit<[u8; { size_of::<V>() }]> without introducing dynamic dispatch like #108638 does. This is split out of #108638 so the performance impact of only this change can be measured.

r? @cjgillot

bors · 2023-03-21T21:49:26Z

☔ The latest upstream changes (presumably #109092) made this pull request unmergeable. Please resolve the merge conflicts.

workingjubilee · 2023-03-26T02:29:42Z

@bors try @rust-timer queue

bors · 2023-03-26T02:29:51Z

⌛ Trying commit 8bb14552a001fe563ad6e3da20fc1b1838a0d6ad with merge 013c87b3813eab23d1ebea1d7cb8aac3624f02a2...

bors · 2023-03-26T04:30:59Z

☀️ Try build successful - checks-actions
Build commit: 013c87b3813eab23d1ebea1d7cb8aac3624f02a2 (013c87b3813eab23d1ebea1d7cb8aac3624f02a2)

rust-timer · 2023-03-26T07:23:00Z

Finished benchmarking commit (013c87b3813eab23d1ebea1d7cb8aac3624f02a2): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.4%, 0.7%]	6
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.4%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.6%, -0.4%]	6

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[0.9%, 4.3%]	3
Improvements ✅ (primary)	-0.9%	[-1.5%, -0.5%]	13
Improvements ✅ (secondary)	-1.5%	[-5.3%, -0.4%]	18
All ❌✅ (primary)	-0.9%	[-1.5%, -0.5%]	13

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[0.9%, 2.0%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-	-	0

compiler/rustc_middle/src/query/erase.rs

cjgillot · 2023-03-26T08:44:50Z

compiler/rustc_query_impl/src/on_disk_cache.rs

@@ -1089,7 +1090,7 @@ pub fn encode_query_results<'a, 'tcx, Q>(

            // Encode the type check tables with the `SerializedDepNodeIndex`
            // as tag.
-            encoder.encode_tagged(dep_node, value);
+            encoder.encode_tagged(dep_node, &Q::restore(*value));


Does this deserve an restore_ref(&Erased<T>) -> &T helper?
Can't we use restore directly?

That's not safe as alignment can differ.

compiler/rustc_query_impl/src/plumbing.rs

cjgillot · 2023-03-26T08:48:06Z

compiler/rustc_query_impl/src/plumbing.rs

-    ([]) => {{
-        Some(dep_graph::hash_result)
+    ([][$V:ty]) => {{
+        Some(|hcx, result| dep_graph::hash_result(hcx, &restore::<$V>(*result)))


Likewise restore_ref

compiler/rustc_middle/src/query/erase.rs

cjgillot · 2023-03-26T09:00:51Z

compiler/rustc_query_impl/src/lib.rs

@@ -44,6 +46,14 @@ pub use on_disk_cache::OnDiskCache;
 mod profiling_support;
 pub use self::profiling_support::alloc_self_profile_query_strings;

+trait QueryToConfig<'tcx>: 'tcx {


Could you document this trait?

cjgillot · 2023-03-26T09:03:43Z

compiler/rustc_query_system/src/query/plumbing.rs

@@ -11,7 +11,6 @@ use crate::query::job::QueryLatch;
 use crate::query::job::{report_cycle, QueryInfo, QueryJob, QueryJobId, QueryJobInfo};
 use crate::query::SerializedDepNodeIndex;
 use crate::query::{QueryContext, QueryMap, QuerySideEffects, QueryStackFrame};
-use crate::values::Value;


The trait and its can now be grouped in rustc_middle::query::values.

Maybe add an impl<T, C> !Value<C> for Erased<T> to avoid jokes?

I ran into specialization errors trying to remove the unsafe Value impls in rustc_middle, so I think I'll just leave it in rustc_query_system.

cjgillot · 2023-03-26T09:07:42Z

compiler/rustc_query_system/src/query/config.rs

@@ -45,6 +45,12 @@ pub trait QueryConfig<Qcx: QueryContext>: Copy {

    fn loadable_from_disk(self, qcx: Qcx, key: &Self::Key, idx: SerializedDepNodeIndex) -> bool;

+    fn from_cycle_error(


Suggested change

fn from_cycle_error(

/// Synthesize a dummy return value to let compilation continue emitting errors.

fn value_from_cycle_error(

Zoxc · 2023-03-26T09:13:50Z

This could use another perf run to see if removing the Debug impl for Erased affects performance.

cjgillot · 2023-03-26T09:14:55Z

@bors try @rust-timer queue

bors · 2023-03-26T09:15:04Z

⌛ Trying commit 38bc74365eacf98fe04b6332643d436b296f17b4 with merge 6804bca3e0eba91188cf84b898e9ac4805993725...

bors · 2023-03-26T11:19:38Z

☀️ Try build successful - checks-actions
Build commit: 6804bca3e0eba91188cf84b898e9ac4805993725 (6804bca3e0eba91188cf84b898e9ac4805993725)

bors · 2023-03-26T11:19:38Z

☀️ Try build successful - checks-actions
Build commit: 6804bca3e0eba91188cf84b898e9ac4805993725 (6804bca3e0eba91188cf84b898e9ac4805993725)

rust-timer · 2023-03-26T12:35:27Z

Finished benchmarking commit (6804bca3e0eba91188cf84b898e9ac4805993725): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.1%, 0.4%]	4
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.3%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.4%, -0.3%]	4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-1.7%	[-3.3%, -0.8%]	62
Improvements ✅ (secondary)	-2.9%	[-4.5%, -1.7%]	5
All ❌✅ (primary)	-1.7%	[-3.3%, -0.8%]	62

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	12.5%	[11.0%, 13.9%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Zoxc · 2023-04-06T07:29:30Z

Rebased.

cjgillot · 2023-04-06T11:22:32Z

@bors r+

bors · 2023-04-06T11:22:34Z

📌 Commit 0110073 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-04-06T16:29:38Z

⌛ Testing commit 0110073 with merge f5b8f44...

bors · 2023-04-06T18:48:35Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing f5b8f44 to master...

rust-timer · 2023-04-06T20:04:36Z

Finished benchmarking commit (f5b8f44): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.6%]	43
Regressions ❌ (secondary)	0.4%	[0.3%, 0.7%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.6%]	43

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-2.7%, -0.6%]	14
Improvements ✅ (secondary)	-2.7%	[-4.9%, -0.8%]	20
All ❌✅ (primary)	-1.3%	[-2.7%, -0.6%]	14

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-2.7%, -2.7%]	1
All ❌✅ (primary)	-	-	0

nnethercote · 2023-04-12T04:53:01Z

@Zoxc: this ended up regressing much more than the pre-merge runs suggested. I see you tried to improve things in #110057, but this didn't work. What now?

Zoxc · 2023-04-14T21:06:16Z

Looking at the non-significant earlier perf results there are some bumps in instructions count when adding the format_value argument to incremental_verify_ich and when fully erasing the value types. The latter would cause inlining differences and is hard to control for. It the may also swing back on further changes. The former is necessary, but it could be a better idea to pass query to it instead. That's something that could be tested post #108638.

I also noticed functions not getting inlined since we use 16 CGUs instead of 1. It would be nice if we had 1 CGU for perf runs.

rylev · 2023-04-19T07:51:21Z

@Zoxc (I'm doing perf triage) would it make sense to make an issue for this? It looks like this has a risk of getting forgotten especially if it takes #108638 a while to land.

Zoxc · 2023-04-19T21:14:27Z

It's not clear if it will be an improvement and it could easily get lost in the sea of issues instead.

rustbot assigned cjgillot Mar 19, 2023